New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin): add no-unnecessary-condition rule #699
Conversation
packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
Outdated
Show resolved
Hide resolved
a0fc766
to
a3a5bf1
Compare
Adds a no-unnecessary-condition, a rule that uses type information to determine if a condition is necessary. Any condition that's indicated by types to always be truthy or falsy is considered to be unnecessary. Essentially a stronger version of no-constant-condition from core eslint. For example: ```ts const items = ['foo', 'bar']; if(items) { } // Error: items is always truthy ```
Since no-unnecessary-conditions and strict-boolean-expressions play similar roles but with different levels of strictness, mark them as related to each other in their docs
Is there anything blocking this from merging? It's had some review and I don't think any action items were raised. I'd love this to go in so that I can use it, and also because this PR has been passively accumulating maintenance items (prettier upgrade, linter rules change, |
It's been sitting at the top of my queue to review. I was also spending some time mulling over the question of whether or not this should just exist as part of |
One question @Retsam - do you think it makes sense to add in support for enum Foo {
a = 1,
b = 2
}
const x: Foo.a = Foo.a;
if (x === Foo.a) {} // lint error
const y = 1;
if (y === 1) {} // lint error |
There's enough overlap in intent and structure with I couldn't summarize the purpose of the combined rule in a sentence, which seems a reasonable heuristic that there's maybe not as much overlap in intent as there may appear on first glance. Plus the options for On the other hand, the |
I'll take a look at the equality checking idea. It definitely seems like it's in scope for this rule's purpose. And thanks for taking another look at this; I know it's volunteer time, and it's appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code overall LGTM - thanks for your contribution.
A few minor comments.
LMK your thoughts on the equality piece when you've had a chance to think about it :)
If both sides of a boolean expression are literals, the condition is unnecessary
I added support for the equality operators (and also the relational operators): I checked if both sides of the operator are literal values, if they are then the expression is unnecessary. The only other case of an unnecessary equality operator I could think of is comparing two entirely disjoint types, e.g. a I gave this case a generic error message "both sides of the expression are literal values" - in theory we could use the same existing error messages ("always truthy" or "always falsy"), but the code to determine which to use in each case was annoyingly verbose and a bit type unfriendly. I can still go that route if you think it's worth doing. I'm also fine punting the equality case to a different PR if you'd prefer to review it separately. (Easy to drop that commit from this PR and move it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also fine punting the equality case to a different PR if you'd prefer to review it separately.
No need - github lets you compare across commits within a PR, so it's super simple to review the new changes.
This is why I always tell people off for force pushing to PR branches, because github doesn't track changes across a force push.
The new changes LGTM.
A few very minor nits, and then I'm happy!
Thanks for working on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(was supposed to RC woops)
Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Codecov Report
@@ Coverage Diff @@
## master #699 +/- ##
==========================================
+ Coverage 94.08% 94.13% +0.04%
==========================================
Files 114 115 +1
Lines 4956 4996 +40
Branches 1382 1393 +11
==========================================
+ Hits 4663 4703 +40
Misses 166 166
Partials 127 127
|
Adds a no-unnecessary-condition, a rule that uses type information to determine if a condition is necessary. Any condition that's indicated by types to always be truthy or falsy is considered to be unnecessary. Essentially a stronger version of no-constant-condition from core eslint.
For example:
The code is largely adapted from
strict-boolean-expressions
: withtslint
, I believe you can get fairly similar behavior to this rule by using theirstrict-boolean-expressions
rule by turning on a lot of options ("allow-null-union", "allow-string", "allow-enum", "allow-number", "allow-boolean-or-undefined"); I was originally going to add some of those options to this codebases version ofstrict-boolean-expressions
, but I believe this just made more sense as its own rule, both in terms of implementation and usage.